Instead of creating a temporary byte array, could we do something like this?

template.body( Request.Body.of(os -> jacksonJaxbJsonProvider.writeTo(
          object, bodyType.getClass(), null, null, APPLICATION_JSON_TYPE, null, os) ) );

This would require a Body.of method that would take an interface that can write body content to an arbitrary stream, so I'm thinking that maybe what I'm suggesting would be some future improvement? If so, then I understand why it is done this way for now.

While I like the elegance of that suggestion, I find three reasons to keep it as is:

1. In vast majority of cases, XML/JSON encoders operate with small chunk of data fitting in-memory.
2. It appears that some clients, servers and the Logger might benefit from Content-Length header. And to calculate the length, the data should be eagerly encoded into tangible bytes one way or another.
3. If there's retry feature enabled, jacksonJaxbJsonProvider.writeTo will be executed two or more times which is not efficient.

Additionally, I like your idea from the other thread to add support for raw OutputStream and Writer bodies (not implemented in this PR). If there's a really big piece of XML or JSON document, users could leverage raw body supplier to stream the large payload:

interface Client {
    @RequestLine("POST /api/v1/send")
    @Header("Content-Type: application/json; charset=UTF-8")
    void send(ThrowingConsumer<OutputStream, IOException> body);
}

client.send(outputStream -> jacksonJaxbJsonProvider.writeTo(..., outputStream));

Alternatively, users can provide custom Encoder implementation with lazy encoding configured.

I'd be happy to hear maintainers' opinion too, and I'm open to any idea.

TL;DR I prefer to keep existing encoders as is and let users enable streaming on demand if needed.

I agree with you about most json encoders working on relatively small chunks. And I agree that there is really no need to make changes to the existing encoders.

You make a very good point about logging - Feign users rely on body logging, and having the body content in memory makes logging a lot simpler. I actually explored logging with streams in my initial prototypes for this project using buffered streams and mark/reset, but it was overly complicated and did not add sufficient value. Your design is much better.

Note that there is a "streaming" encoder for Feign that looks like it is designed to handle very large json streams, and they might benefit from this (I haven't dug into that encoder in depth), but for the general case, I am in agreement that we should leave the existing encoders alone until there is a compelling reason to change them.

I guess I should ask why we are dropping Serializable? Personally, I don't think it should have been there in the first place, but removing it adds more to the breaking change footprint. Is there any disadvantage to leaving it?

I have no clue why it was Serializable without a serialVersionUID. I don't find any benefit of keeping it since feign.Request#body is no longer serializable

I hope the maintainers will let us know if it's necessary to keep it

I completely agree in principle (that should never have passed code review when it was originally written).

My concern is that this might create more friction to us getting the PR accepted - and it's not relevant to the big picture we are trying to achieve.

On the flip side, if you are making a breaking change, it's best to break everything that needs to be broken at the same time :-) We'll see what they say.

@trumpetinc I found this thread - #1193

It seems that we need to either redesign FeignException as part of the major release or keep Request serializable (does it have any value without the body?)

Ugh, Java serialization was such a broken design.

And the fact that Feign uses the response body field in FeignException to store request bodies is super awkward. FeignException should have been an abstract base class with different subclasses for requests vs responses.

But that is not our problem to solve.

Here is my proposal - do you think this is a reasonable compromise?

Make FeignExceptiom serializable again (and include a comment about explaining why with link back to the issue you found).

Change the "request" constructor so it accepts Body. It will then call toString() like the Feign logger does. Then convert the string to byte[] using UTF_8 encoding.

And include comments explaining that we know this is weird, but we are having to do this to work around FeignException not having an abstract base class with a separate class for request exceptions vs response exceptions.

Decided not to break the backward compatibility for FeignException for now. Made the Request serializable again, but the Request#body & RequestTemplate#body are transient now. I'd suggest revising the exception handling in a separate PR

You may want to consider not including this method right now. It is a little confusing having a pure byte oriented call that implicitly involves charsets.

I couldn't find any callers of this, and given potential for confusion, it is probably safest to just not include it.

It's in use by some encoders (DefaultEncoder, JacksonEncoder, JacksonJaxbJsonEncoder)

JSON encoders should be fine with implicit UTF-8 charset. Otherwise, I need to update the encoding logic (encode to String) which I'm reluctant to do tbh

BodyImpl needs the charset for toString()/logging purposes only. If a given byte[] is not a human-readable sequence of bytes, then the caller shouldn't even bother logging the body (=providing any charset)

oh weird - my "Callers" search seems to be failing. Sorry about that. Totally agree with leaving it as-is. It is a point of confusion, but not worth changing right now.

I have a similar question about the need to expose this as public API. I think the of(xxx) methods are primarily about backwards compatibility for character based request bodies, in which case only of(String content, Charset charset) and of(String content) are needed.

I would move the new Request.BodyImpl(...) code into the of(String, Charset) method and remove both of(byte[]....) calls.

very small thing, and probably not worth worrying about, but I prefer asString(chaset) and asByteArray().

Usually writeToXXX() implies that we will pass XXX in as a target to the method (which isn't the case here or in the writeToByteArray() method).

It actually was asString at some point. I decided to move to writeToString to emphasize that this is an IO operation, so that asString is not confused with toString, especially considering that the body might be non-repeatable.

I'm now thinking that writeAsString might be a good alternative

Please let me know what option you prefer and I update accordingly

Ah - good point - the potential for it being an IO operation (and throwing IOException) does make it not quite as clear...

I'm fine with either naming - this was a minor item and I don't feel that strongly about it...

consider byte[] asByteArray()

This seems like it is more than just backwards compatibility. Can you expand on why these changes are being made? I'm not opposed - I would just like to understand what's going on.

So JDK HTTP Client takes InputStream as request body content. In order to adapt feign.Request.Body to InputStream, I need to leverage a combination of piped input/output streams.

If I just make

BodyPublisher publisher =
        BodyPublishers.ofInputStream(
            () -> {
              PipedInputStream inputStream = new PipedInputStream();
              try (PipedOutputStream outputStream = new PipedOutputStream(inputStream)) {
                body.writeTo(outputStream);
              } catch (IOException e) {
                throw new UncheckedIOException(e);
              }
              return inputStream;
            });

then:

1. The whole request body will be cached in-memory.
2. return inputStream will get blocked until the body is fully written to the outputStream.

To kill two birds with one stone, we need to execute body.writeTo(outputStream) asynchronously and therefore PropagatingPipedInputStream class & feign.http2client.Http2Client#executor instance variable were introduced.

Http2Client(HttpClient) constructor must be sufficient for most users. ForkJoinPool#commonPool is created & managed by JVM (no user intervention is required). By default, the same pool is used when executing CompletableFuture#runAsync(Runnable) while offering CompletableFuture#runAsync(Runnable, Executor) alternative.

Similarly, in case if anyone wants to supply & manage the executor on their own, they can utilize Http2Client(HttpClient, Executor) constructor.

Thus, I kept the constructor to provide both backward compatibility and convenience

I just did a dive into HttpClient BodyPublisher, and I understand now. It's unfortunate that they made the design decision they did (it's a similar decision to the original Feign design that we are trying to fix with this PR).

Thanks for the explanation. It does make sense that we'd need to do this to properly support streaming.